Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Build][Docs][CI] BUILD_FUNCTIONAL_TESTS=False by default #557

Conversation

hjabird
Copy link
Contributor

@hjabird hjabird commented Aug 16, 2024

Description

  • Currently tests build by default
    • This adds additional dependencies, tripping users up.
    • This slows down builds where the tests are not used.
  • This PR disabled tests by default and
    • Updates the documentation
    • Updates the Github CI

Fixes #443

Checklist

All Submissions

  • [N/A] Do all unit tests pass locally? Attach a log.
  • [N/A] Have you formatted the code using clang-format?

* Currently tests build by default
  * This adds additional dependencies, tripping users up.
  * This slows down builds where the tests are not used.
* This PR disabled tests by default and
  * Updates the documentation
  * Updates the Github CI
@hjabird hjabird marked this pull request as ready for review August 16, 2024 11:17
.github/workflows/pr.yml Outdated Show resolved Hide resolved
@Rbiessy Rbiessy requested a review from a team August 16, 2024 12:42
Co-authored-by: Romain Biessy <romain.biessy@codeplay.com>
@@ -64,7 +64,7 @@ set(ONEMKL_SYCL_IMPLEMENTATION "dpc++" CACHE STRING "Name of the SYCL compiler")
set(HIP_TARGETS "" CACHE STRING "Target HIP architectures")

## Testing
option(BUILD_FUNCTIONAL_TESTS "" ON)
option(BUILD_FUNCTIONAL_TESTS "" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this extra Netlib dependency causes problems for unexperienced users, so it makes sense to disable it, but I think we still need some default validation to make sure the build is fine. As far as I know extra dependency is a problem only for BLAS and LAPACK, does it make sense to keep testing for other domains since they don't have extra dependencies? And for BLAS/LAPACK we can come up with more light-weight testing in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about a default validation for unexperienced users? The examples are still built by default and don't require any external dependencies. This should be enough to make sure everything built fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I can agree that passed examples are enough to confirm that built oneMKL Interfaces functionality is fine, we cover just 1-2 functions per domain, only USM API and mostly Intel oneMKL CPU backend for compiler time dispatcher from the build and we do not check correctness which could be a tricky problem to catch in the application where this oneMKL Interfaces build will be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying the examples are enough to test all the functionalities, just that they are enough to show oneMKL built. The point of this PR is that we have seen users getting confused why they couldn't build oneMKL and the issue was due to the additional dependencies needed by the tests. This PR aims to clarify this and force the users to manually enable the tests if they want them.
I think this makes it easier for first time users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gentle ping @mkrainiuk. What are your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support the idea of disabling testing for BLAS and LAPACK for the first time users and come up with some light testing that won't require Netlib package in the future, but I don't understand why we need to disable testing for DFT, RNG, and spBLAS that have proper testing coverage by default and don't have any extra dependencies that might confuse users. Could you please clarify why it's required to change testing mode for these domains as part of Netlib dependency problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is much simpler for users and developers if a CMake option always has the same default value. Some people use CMake GUI tools which show all the options with the default value but this causes confusions when the value change due to another option. We would need to add some documentation to explain when the tests are enabled or disabled and hope users find it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, in this case I don't think modifying common option will be the right solution, instead it will be better to change requirements for Netlib dependency in two specific domains, where it's used, we can make it optional and based on the Netlib availability enable/disable unit tests where Netlib is used (later on adding some tests where it's not used so they can be built without Netlib).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be more specific here is a draft with some quick changes for what I've proposed before as potential solution for Netlib dependency problem #577
Please let me know what do you think about this approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. Your solution looks fine to me!

@Rbiessy
Copy link
Contributor

Rbiessy commented Oct 16, 2024

Closing in favor of #577

@Rbiessy Rbiessy closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CMake] New users can struggle with the CBLAS testing dependency when building oneMKL - what to do?
3 participants